chore(warnings): resolve 25 analyzer quick wins and fix RAPS bugs#261
chore(warnings): resolve 25 analyzer quick wins and fix RAPS bugs#261rlorenzo wants to merge 8 commits into
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
Bundle ReportBundle size has no change ✅ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #261 +/- ##
==========================================
+ Coverage 45.39% 45.59% +0.20%
==========================================
Files 916 916
Lines 52784 52806 +22
Branches 5005 5014 +9
==========================================
+ Hits 23960 24076 +116
+ Misses 28196 28097 -99
- Partials 628 633 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
📝 WalkthroughWalkthroughThe PR adds RAPS request validation and scoped background synchronization, fixes group-update status handling, consolidates Directory routing and search logic, expands controller tests, refactors test wiring and CMS logging, and updates analyzer suppressions. ChangesRAPS group management
Directory routing and search
Test and controller wiring
Diagnostic suppression maintenance
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RAPSController
participant IServiceScopeFactory
participant RAPSContext
participant OuGroupService
participant NLog
RAPSController->>RAPSController: Validate GroupSync ModelState
RAPSController->>IServiceScopeFactory: CreateScope()
IServiceScopeFactory->>RAPSContext: Resolve fresh context
RAPSController->>OuGroupService: Sync group
OuGroupService-->>RAPSController: Complete or throw
RAPSController->>NLog: Log synchronization failure
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/Areas/Directory/Controllers/DirectoryController.cs`:
- Line 35: Update the Directory home action route in DirectoryController to
preserve the trailing slash, using an area-root route that resolves to
/Directory/; alternatively, add a redirect from /Directory to /Directory/.
Ensure relative "nav" requests continue resolving under the Directory path.
In `@web/Areas/RAPS/Controllers/RAPSController.cs`:
- Around line 94-99: Update the read-only queries in Nav, specifically the
TblRoles, TblPermissions, and VwAaudUser lookups, to use AsNoTracking() before
execution while preserving their existing filters and null-handling behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b12c4003-6bfe-4fe7-9c83-98cd8619e44c
📒 Files selected for processing (9)
.editorconfigtest/ClinicalScheduler/Integration/ServiceLayerIntegrationTest.cstest/RAPS/AdGroupsControllerTests.csweb/Areas/CMS/Controllers/CMSController.csweb/Areas/CTS/Controllers/CTSController.csweb/Areas/Directory/Controllers/DirectoryController.csweb/Areas/RAPS/Controllers/AdGroupsController.csweb/Areas/RAPS/Controllers/RAPSController.csweb/Areas/RAPS/Models/GroupAddEdit.cs
0375aea to
bd4a19f
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
This PR is the first tranche of the .NET analyzer-warning cleanup plan for VIPER’s ASP.NET backend, combining low-risk warning fixes/suppressions with a few behavior-correctness fixes discovered during the cleanup (notably in RAPS and one API bug).
Changes:
- Fixes several real behavior bugs (e.g., AdGroups PUT ID mismatch fallthrough; RAPS
SingleAsync->SingleOrDefaultAsync; RAPS action pipeline double-execution; request-scoped DbContext capture in background sync). - Adds ModelState guards to multiple RAPS actions and uses targeted analyzer suppressions/
[NonAction]where appropriate. - Refactors a test constructor to reduce complexity and adds a regression test for the AdGroups update bug.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| web/Areas/RAPS/Models/GroupAddEdit.cs | Makes GroupId nullable to better represent “new group” semantics and satisfy analyzer guidance. |
| web/Areas/RAPS/Controllers/RAPSController.cs | Fixes filter pipeline double-execution, hardens nav member lookup, and moves OU sync to a background scope. |
| web/Areas/RAPS/Controllers/AdGroupsController.cs | Fixes missing return on mismatched PUT ID to prevent unintended updates. |
| web/Areas/Directory/Controllers/DirectoryController.cs | Consolidates route prefix at controller level while keeping endpoints consistent. |
| web/Areas/CTS/Controllers/CTSController.cs | Adds targeted analyzer suppression for filter override. |
| web/Areas/CMS/Controllers/CMSController.cs | Uses ILoggerFactory to create a Data.CMS-category logger without triggering the analyzer. |
| test/RAPS/AdGroupsControllerTests.cs | Adds regression coverage for the mismatched group ID PUT bug. |
| test/ClinicalScheduler/Integration/ServiceLayerIntegrationTest.cs | Extracts mock factories to reduce constructor complexity. |
| .editorconfig | Adds/extends justified suppressions for generated contexts and non-bound model types. |
03058c4 to
d230b4e
Compare
45e0335 to
9a65086
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/Areas/Directory/Controllers/DirectoryController.cs (2)
90-97: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winAvoid nested per-item searches.
Using
.Find()inside a loop over a collection causes O(N*M) time complexity. As per coding guidelines, pre-build aDictionary(orLookup) and use it to look up items within the loop.⚡ Proposed fix
+ var individualsByIamId = individuals + .Where(m => m.IamId != null) + .GroupBy(m => m.IamId) + .ToDictionary(g => g.Key, g => g.First()); + foreach (var l in ldap) { - AaudUser? userInfo = individuals.Find(m => m.IamId == l.IamId); + individualsByIamId.TryGetValue(l.IamId ?? string.Empty, out var userInfo); var result = hasDetailPermission ? new IndividualSearchResultWithIDs(userInfo, l) : new IndividualSearchResult(userInfo, l); result.LookupEmailHost(_aaud); results.Add(result); AddVmacsContactInfo(result); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Areas/Directory/Controllers/DirectoryController.cs` around lines 90 - 97, Replace the per-item individuals.Find call in the ldap iteration with a pre-built Dictionary or Lookup keyed by IamId, created before the loop. Retrieve each AaudUser through that index while preserving the existing result construction, LookupEmailHost, and results.Add behavior.Source: Coding guidelines
61-74: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
.Select()for mapping.
Both methods manually iterate and append items to aListinstead of using LINQ.Select(). As per coding guidelines, use.Select()for mapping rather thanforeach + Add.
web/Areas/Directory/Controllers/DirectoryController.cs#L61-L74: Refactorindividuals.ForEachto map elements using.Select().web/Areas/Directory/Controllers/DirectoryController.cs#L85-L101: Refactorforeach (var l in ldap)similarly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Areas/Directory/Controllers/DirectoryController.cs` around lines 61 - 74, Replace the manual list construction and individuals.ForEach mapping in DirectoryController.cs lines 61-74 with a LINQ Select projection that preserves permission-based result creation, LookupEmailHost, and AddVmacsContactInfo behavior. Apply the same Select-based mapping refactor to the ldap loop in DirectoryController.cs lines 85-101, with no direct changes required beyond converting that loop to projection.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/Areas/Directory/Controllers/DirectoryController.cs`:
- Around line 135-145: Update AddVmacsContactInfo to return async Task and await
VMACSService.Search instead of accessing .Result; then update every caller in
DirectoryController to await this helper and propagate async Task as needed,
preserving the existing result-mapping behavior.
In `@web/Areas/RAPS/Controllers/RAPSController.cs`:
- Around line 302-305: Extract the repeated ModelState.IsValid checks from the
affected RAPSController actions into a shared ValidateModelStateAttribute action
filter that returns BadRequestResult when validation fails. Apply the filter
consistently to all eight affected actions, then remove their inline guards
while preserving the existing action behavior for valid model state.
- Around line 599-631: Update the background-job entry point
SyncGroupInBackground to catch and log every Exception, rather than filtering to
a limited set of exception types. Preserve the existing error context and
groupId in LogManager logging so unexpected failures, including database
exceptions such as SqlException, are observed.
---
Outside diff comments:
In `@web/Areas/Directory/Controllers/DirectoryController.cs`:
- Around line 90-97: Replace the per-item individuals.Find call in the ldap
iteration with a pre-built Dictionary or Lookup keyed by IamId, created before
the loop. Retrieve each AaudUser through that index while preserving the
existing result construction, LookupEmailHost, and results.Add behavior.
- Around line 61-74: Replace the manual list construction and
individuals.ForEach mapping in DirectoryController.cs lines 61-74 with a LINQ
Select projection that preserves permission-based result creation,
LookupEmailHost, and AddVmacsContactInfo behavior. Apply the same Select-based
mapping refactor to the ldap loop in DirectoryController.cs lines 85-101, with
no direct changes required beyond converting that loop to projection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ffb720a2-d3a4-4cd4-9d60-e2daa11f438e
📒 Files selected for processing (11)
.editorconfigtest/ClinicalScheduler/Integration/ServiceLayerIntegrationTest.cstest/Directory/DirectoryControllerTests.cstest/RAPS/AdGroupsControllerTests.cstest/RAPS/RAPSControllerTests.csweb/Areas/CMS/Controllers/CMSController.csweb/Areas/CTS/Controllers/CTSController.csweb/Areas/Directory/Controllers/DirectoryController.csweb/Areas/RAPS/Controllers/AdGroupsController.csweb/Areas/RAPS/Controllers/RAPSController.csweb/Areas/RAPS/Models/GroupAddEdit.cs
9a65086 to
902911b
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/Areas/Directory/Controllers/DirectoryController.cs`:
- Line 87: Replace the linear per-item individuals.Find lookup in the
surrounding directory search flow with a ToLookup-based index created once from
individuals before the loop. Use the lookup inside the loop to retrieve matching
users in O(1), preserving the existing matching key and result behavior.
In `@web/Areas/RAPS/Controllers/RAPSController.cs`:
- Around line 26-27: Remove the unused Count and UserName properties from the
RAPSController class, leaving the remaining controller state and behavior
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a1efe47d-ff84-4d5d-b8b1-9564bdc5e7e3
📒 Files selected for processing (4)
test/Directory/DirectoryControllerTests.cstest/RAPS/RAPSControllerTests.csweb/Areas/Directory/Controllers/DirectoryController.csweb/Areas/RAPS/Controllers/RAPSController.cs
902911b to
7012aa4
Compare
- suppress CA1505 for scaffolded EF contexts and S6964 for ApiPagination, which is never model-bound (editorconfig precedent) - create the Data.CMS logger via ILoggerFactory so the logger category matches its consumer (S6672) - move the Directory route prefix to the controller, URLs unchanged (S6931) - return 400 from RAPS view actions when model binding fails instead of proceeding with defaulted params (S6967) - suppress S6967/S6932 on the RAPS and CTS nav filter overrides with justification; mark RAPSController.Nav [NonAction] to remove an unintended endpoint - make GroupAddEdit.GroupId nullable to reflect create semantics (S6964)
Splits the two large NSubstitute fakes out of ServiceLayerIntegrationTest constructor (CA1502); behavior unchanged.
The BadRequest result was discarded, so a PUT whose body groupId did not match the route fell through and updated the group anyway. Add a regression test asserting the 400 and that the group is unchanged.
SingleAsync threw on every RAPS page when the memberId query param matched no user; the nav already handles the null case.
OnActionExecutionAsync awaited both the AreaController base, which already invokes next(), and next() again, so every RAPS action ran its pipeline twice.
The fire-and-forget sync captured the request-scoped RAPSContext, which is disposed once the response returns, faulting the background task. Resolve a fresh context from IServiceScopeFactory and log failures instead of leaving them unobserved.
Unit-locks the new 400-on-binding-failure behavior for all 8 guarded RAPS view actions and covers the permission lookup found/not-found paths plus the GroupSync no-op path (no background sync for unknown groups). Raises the PR patch coverage flagged by Codecov.
- Closes the CA1502 warning on GetUCD; the shared query helper and the VMACS enrichment were duplicated verbatim across both actions - SQLite-backed tests pin the query helper (field matches, name-span match, current-user filter, ordering)
7012aa4 to
7ac4816
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/Areas/Directory/Controllers/DirectoryController.cs`:
- Around line 108-114: Update DirectoryResult to return the synchronous View
result directly without wrapping it in Task.Run, adjusting the method signature
as needed to match its synchronous behavior. Rename the XML parameter
documentation and inline comment references from uid to mothraID.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 01f93fff-c33a-48ec-9153-c68e6a343460
📒 Files selected for processing (11)
.editorconfigtest/ClinicalScheduler/Integration/ServiceLayerIntegrationTest.cstest/Directory/DirectoryControllerTests.cstest/RAPS/AdGroupsControllerTests.cstest/RAPS/RAPSControllerTests.csweb/Areas/CMS/Controllers/CMSController.csweb/Areas/CTS/Controllers/CTSController.csweb/Areas/Directory/Controllers/DirectoryController.csweb/Areas/RAPS/Controllers/AdGroupsController.csweb/Areas/RAPS/Controllers/RAPSController.csweb/Areas/RAPS/Models/GroupAddEdit.cs
Summary
First branch of the .NET warnings cleanup plan: closes 26 of 44 code-analysis warnings via low-risk fixes and justified suppressions, plus four real bugs found along the way (three in RAPS, one latent test hazard). Seven single-purpose commits.
Warning cleanup (26 closed)
[NonAction]onRAPSController.Navremoves an unintended endpoint.editorconfigsuppression forApiPagination(never model-bound, always constructed server-side);GroupAddEdit.GroupIdmade nullable.editorconfigsuppression for scaffolded EF*Context.cs, following the existing S3251 precedentILoggerFactoryforData.CMS; log category unchanged[Route]; all 5 URLs provably unchangedServiceLayerIntegrationTestGetUCD)GetandGetUCD, extracted into shared helpers; SQLite-backed tests pin the query helper (field matches, name-span match, current-user filter, ordering)Bug fixes (behavior changes to note)
AdGroupsController.UpdateGroup: missingreturnmeant a PUT with a mismatched group ID fell through and updated the group anyway; now 400. Regression test added (the group-edit UI always sends a matching ID, so normal use is unaffected).SingleAsync500ed every RAPS page when?memberId=matched no user; nowSingleOrDefaultAsyncwith the existing null handling.OnActionExecutionAsyncawaited both theAreaControllerbase (which already invokesnext()) andnext()again.GroupSyncbackground sync: fire-and-forget task captured the request-scopedRAPSContext(disposed when the response returns); now resolves a fresh context fromIServiceScopeFactoryand logs failures.Verification
verify:build: all 26 targeted warnings gone, no new ones; the 18 remaining warnings match the plan's deferred list exactly?roleId=abc400, nonexistent?memberId=200), mismatched-ID group PUT 400, all 5 Directory endpoints, CTS index